-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update v44 pricefeed addresses to use bytes #1053
Conversation
string base_asset = 2; | ||
string quote_asset = 3; | ||
repeated bytes oracles = 4 [ | ||
(cosmos_proto.scalar) = "cosmos.AddressBytes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know of any docs on what the (cosmos_proto.scalar)
does? I haven't been able to find any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to be used yet, maybe later in cosmos v0.45. Just added to be consistent with swap module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Couple of additional requests:
- Do you want to separate the proto query types from the store types. See Spike split proto types #1050 and the swap query types.
- There's a TODO in
module.go
for registering invariants. The pricefeed module doesn't have any invariants so we can remove the TODO. - Minor code quality improvements:
CalculateMedianPrice
andcalculateMeanPrice
are just math functions, so they don't need access to the ctx, so that argument could be removed. AlsocalculateMeanPrice
will panic if anyone ever passes in a slice of more than 2 prices. I'd probably swap the slice arg for two individual prices args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.